[LLD][COFF] Follow up comments on pr146610 #147152
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-lld Author: Alexandre Ganea (aganea) ChangesThis is a follow-up PR for post-commit comments in #146610
Full diff: https://github.com/llvm/llvm-project/pull/147152.diff 5 Files Affected:
diff --git a/lld/COFF/Config.h b/lld/COFF/Config.h
index 79b63e5b7236f..9220c847f356d 100644
--- a/lld/COFF/Config.h
+++ b/lld/COFF/Config.h
@@ -307,7 +307,7 @@ struct Configuration {
bool warnDebugInfoUnusable = true;
bool warnLongSectionNames = true;
bool warnStdcallFixup = true;
- bool warnExportedDllMain = true;
+ bool warnImportedDllMain = true;
bool incremental = true;
bool integrityCheck = false;
bool killAt = false;
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 283aeed1a19cd..0f649ff1cf4f8 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -1643,8 +1643,8 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
config->warnLocallyDefinedImported = false;
else if (s == "longsections")
config->warnLongSectionNames = false;
- else if (s == "exporteddllmain")
- config->warnExportedDllMain = false;
+ else if (s == "importeddllmain")
+ config->warnImportedDllMain = false;
// Other warning numbers are ignored.
}
}
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 0b7dbea8cdd99..1edd24cdf2dea 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -128,13 +128,13 @@ static bool fixupDllMain(COFFLinkerContext &ctx, llvm::object::Archive *file,
file->getFileName() +
": could not get the buffer for a child buffer of the archive");
if (identify_magic(mb.getBuffer()) == file_magic::coff_import_library) {
- if (ctx.config.warnExportedDllMain) {
+ if (ctx.config.warnImportedDllMain) {
// We won't place DllMain symbols in the symbol table if they are
// coming from a import library. This message can be ignored with the flag
- // '/ignore:exporteddllmain'
+ // '/ignore:importeddllmain'
Warn(ctx)
<< file->getFileName()
- << ": skipping exported DllMain symbol [exporteddllmain]\nNOTE: this "
+ << ": skipping imported DllMain symbol [importeddllmain]\nNOTE: this "
"might be a mistake when the DLL/library was produced.";
}
skipDllMain = true;
@@ -207,10 +207,10 @@ void ArchiveFile::parse() {
// Read the symbol table to construct Lazy objects.
bool skipDllMain = false;
for (const Archive::Symbol &sym : file->symbols()) {
- // If the DllMain symbol was exported by mistake, skip importing it
- // otherwise we might end up with a import thunk in the final binary which
- // is wrong.
- if (sym.getName() == "__imp_DllMain" || sym.getName() == "DllMain") {
+ // If an import library provides the DllMain symbol, skip importing it, as
+ // we should be using our own DllMain, not another DLL's DllMain.
+ if (sym.getName() == "__imp_DllMain" || sym.getName() == "DllMain" ||
+ sym.getName() == "_DllMain") {
if (fixupDllMain(ctx, file.get(), sym, skipDllMain))
continue;
}
diff --git a/lld/test/COFF/imported-dllmain-i386.test b/lld/test/COFF/imported-dllmain-i386.test
new file mode 100644
index 0000000000000..a9cd94d089e29
--- /dev/null
+++ b/lld/test/COFF/imported-dllmain-i386.test
@@ -0,0 +1,59 @@
+REQUIRES: x86
+RUN: split-file %s %t.dir && cd %t.dir
+
+RUN: llvm-mc -filetype=obj -triple=i386-windows a.s -o a.obj
+
+RUN: llvm-mc -filetype=obj -triple=i386-windows b1.s -o b1.obj
+RUN: llvm-mc -filetype=obj -triple=i386-windows b2.s -o b2.obj
+
+### This is the line where our problem occurs. Here, we export the DllMain symbol which shouldn't happen normally.
+RUN: lld-link b1.obj b2.obj -out:b.dll -dll -implib:b.lib -entry:DllMain -export:bar -export:DllMain -safeseh:no
+
+RUN: llvm-mc -filetype=obj -triple=i386-windows c.s -o c.obj
+RUN: lld-link -lib c.obj -out:c.lib
+
+### Later, if b.lib is provided before other libs/objs that export DllMain statically, we previously were using the dllimported DllMain from b.lib, which is wrong.
+RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain -safeseh:no 2>&1 | FileCheck -check-prefix=WARN %s
+RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain -ignore:importeddllmain -safeseh:no 2>&1 | FileCheck -check-prefix=IGNORED --allow-empty %s
+RUN: llvm-objdump --private-headers -d out.dll | FileCheck -check-prefix=DISASM %s
+
+WARN: lld-link: warning: b.lib: skipping imported DllMain symbol [importeddllmain]
+IGNORED-NOT: lld-link: warning: b.lib: skipping imported DllMain symbol [importeddllmain]
+
+DISASM: The Import Tables:
+DISASM: DLL Name: b.dll
+DISASM-NOT: DllMain
+DISASM: bar
+DISASM: Disassembly of section .text:
+DISASM-EMPTY:
+DISASM-NEXT: b0 01 movb $0x1, %al
+DISASM-NEXT: c3 retl
+
+#--- a.s
+ .text
+ .globl _foo
+_foo:
+ call *__imp__bar
+ ret
+
+#--- b1.s
+ .text
+ .globl _bar
+_bar:
+ ret
+
+#--- b2.s
+ .intel_syntax noprefix
+ .text
+ .globl _DllMain
+_DllMain:
+ xor al, al
+ ret
+
+#--- c.s
+ .intel_syntax noprefix
+ .text
+ .globl _DllMain
+_DllMain:
+ mov al, 1
+ ret
diff --git a/lld/test/COFF/exported-dllmain.test b/lld/test/COFF/imported-dllmain.test
similarity index 86%
rename from lld/test/COFF/exported-dllmain.test
rename to lld/test/COFF/imported-dllmain.test
index fcf6ed1005379..fa8579b1b41c5 100644
--- a/lld/test/COFF/exported-dllmain.test
+++ b/lld/test/COFF/imported-dllmain.test
@@ -14,11 +14,11 @@ RUN: lld-link -lib c.obj -out:c.lib
### Later, if b.lib is provided before other libs/objs that export DllMain statically, we previously were using the dllimported DllMain from b.lib, which is wrong.
RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain 2>&1 | FileCheck -check-prefix=WARN %s
-RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain -ignore:exporteddllmain 2>&1 | FileCheck -check-prefix=IGNORED --allow-empty %s
+RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain -ignore:importeddllmain 2>&1 | FileCheck -check-prefix=IGNORED --allow-empty %s
RUN: llvm-objdump --private-headers -d out.dll | FileCheck -check-prefix=DISASM %s
-WARN: lld-link: warning: b.lib: skipping exported DllMain symbol [exporteddllmain]
-IGNORED-NOT: lld-link: warning: b.lib: skipping exported DllMain symbol [exporteddllmain]
+WARN: lld-link: warning: b.lib: skipping imported DllMain symbol [importeddllmain]
+IGNORED-NOT: lld-link: warning: b.lib: skipping imported DllMain symbol [importeddllmain]
DISASM: The Import Tables:
DISASM: DLL Name: b.dll
|
|
@llvm/pr-subscribers-lld-coff Author: Alexandre Ganea (aganea) ChangesThis is a follow-up PR for post-commit comments in #146610
Full diff: https://github.com/llvm/llvm-project/pull/147152.diff 5 Files Affected:
diff --git a/lld/COFF/Config.h b/lld/COFF/Config.h
index 79b63e5b7236f..9220c847f356d 100644
--- a/lld/COFF/Config.h
+++ b/lld/COFF/Config.h
@@ -307,7 +307,7 @@ struct Configuration {
bool warnDebugInfoUnusable = true;
bool warnLongSectionNames = true;
bool warnStdcallFixup = true;
- bool warnExportedDllMain = true;
+ bool warnImportedDllMain = true;
bool incremental = true;
bool integrityCheck = false;
bool killAt = false;
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 283aeed1a19cd..0f649ff1cf4f8 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -1643,8 +1643,8 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
config->warnLocallyDefinedImported = false;
else if (s == "longsections")
config->warnLongSectionNames = false;
- else if (s == "exporteddllmain")
- config->warnExportedDllMain = false;
+ else if (s == "importeddllmain")
+ config->warnImportedDllMain = false;
// Other warning numbers are ignored.
}
}
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 0b7dbea8cdd99..1edd24cdf2dea 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -128,13 +128,13 @@ static bool fixupDllMain(COFFLinkerContext &ctx, llvm::object::Archive *file,
file->getFileName() +
": could not get the buffer for a child buffer of the archive");
if (identify_magic(mb.getBuffer()) == file_magic::coff_import_library) {
- if (ctx.config.warnExportedDllMain) {
+ if (ctx.config.warnImportedDllMain) {
// We won't place DllMain symbols in the symbol table if they are
// coming from a import library. This message can be ignored with the flag
- // '/ignore:exporteddllmain'
+ // '/ignore:importeddllmain'
Warn(ctx)
<< file->getFileName()
- << ": skipping exported DllMain symbol [exporteddllmain]\nNOTE: this "
+ << ": skipping imported DllMain symbol [importeddllmain]\nNOTE: this "
"might be a mistake when the DLL/library was produced.";
}
skipDllMain = true;
@@ -207,10 +207,10 @@ void ArchiveFile::parse() {
// Read the symbol table to construct Lazy objects.
bool skipDllMain = false;
for (const Archive::Symbol &sym : file->symbols()) {
- // If the DllMain symbol was exported by mistake, skip importing it
- // otherwise we might end up with a import thunk in the final binary which
- // is wrong.
- if (sym.getName() == "__imp_DllMain" || sym.getName() == "DllMain") {
+ // If an import library provides the DllMain symbol, skip importing it, as
+ // we should be using our own DllMain, not another DLL's DllMain.
+ if (sym.getName() == "__imp_DllMain" || sym.getName() == "DllMain" ||
+ sym.getName() == "_DllMain") {
if (fixupDllMain(ctx, file.get(), sym, skipDllMain))
continue;
}
diff --git a/lld/test/COFF/imported-dllmain-i386.test b/lld/test/COFF/imported-dllmain-i386.test
new file mode 100644
index 0000000000000..a9cd94d089e29
--- /dev/null
+++ b/lld/test/COFF/imported-dllmain-i386.test
@@ -0,0 +1,59 @@
+REQUIRES: x86
+RUN: split-file %s %t.dir && cd %t.dir
+
+RUN: llvm-mc -filetype=obj -triple=i386-windows a.s -o a.obj
+
+RUN: llvm-mc -filetype=obj -triple=i386-windows b1.s -o b1.obj
+RUN: llvm-mc -filetype=obj -triple=i386-windows b2.s -o b2.obj
+
+### This is the line where our problem occurs. Here, we export the DllMain symbol which shouldn't happen normally.
+RUN: lld-link b1.obj b2.obj -out:b.dll -dll -implib:b.lib -entry:DllMain -export:bar -export:DllMain -safeseh:no
+
+RUN: llvm-mc -filetype=obj -triple=i386-windows c.s -o c.obj
+RUN: lld-link -lib c.obj -out:c.lib
+
+### Later, if b.lib is provided before other libs/objs that export DllMain statically, we previously were using the dllimported DllMain from b.lib, which is wrong.
+RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain -safeseh:no 2>&1 | FileCheck -check-prefix=WARN %s
+RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain -ignore:importeddllmain -safeseh:no 2>&1 | FileCheck -check-prefix=IGNORED --allow-empty %s
+RUN: llvm-objdump --private-headers -d out.dll | FileCheck -check-prefix=DISASM %s
+
+WARN: lld-link: warning: b.lib: skipping imported DllMain symbol [importeddllmain]
+IGNORED-NOT: lld-link: warning: b.lib: skipping imported DllMain symbol [importeddllmain]
+
+DISASM: The Import Tables:
+DISASM: DLL Name: b.dll
+DISASM-NOT: DllMain
+DISASM: bar
+DISASM: Disassembly of section .text:
+DISASM-EMPTY:
+DISASM-NEXT: b0 01 movb $0x1, %al
+DISASM-NEXT: c3 retl
+
+#--- a.s
+ .text
+ .globl _foo
+_foo:
+ call *__imp__bar
+ ret
+
+#--- b1.s
+ .text
+ .globl _bar
+_bar:
+ ret
+
+#--- b2.s
+ .intel_syntax noprefix
+ .text
+ .globl _DllMain
+_DllMain:
+ xor al, al
+ ret
+
+#--- c.s
+ .intel_syntax noprefix
+ .text
+ .globl _DllMain
+_DllMain:
+ mov al, 1
+ ret
diff --git a/lld/test/COFF/exported-dllmain.test b/lld/test/COFF/imported-dllmain.test
similarity index 86%
rename from lld/test/COFF/exported-dllmain.test
rename to lld/test/COFF/imported-dllmain.test
index fcf6ed1005379..fa8579b1b41c5 100644
--- a/lld/test/COFF/exported-dllmain.test
+++ b/lld/test/COFF/imported-dllmain.test
@@ -14,11 +14,11 @@ RUN: lld-link -lib c.obj -out:c.lib
### Later, if b.lib is provided before other libs/objs that export DllMain statically, we previously were using the dllimported DllMain from b.lib, which is wrong.
RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain 2>&1 | FileCheck -check-prefix=WARN %s
-RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain -ignore:exporteddllmain 2>&1 | FileCheck -check-prefix=IGNORED --allow-empty %s
+RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain -ignore:importeddllmain 2>&1 | FileCheck -check-prefix=IGNORED --allow-empty %s
RUN: llvm-objdump --private-headers -d out.dll | FileCheck -check-prefix=DISASM %s
-WARN: lld-link: warning: b.lib: skipping exported DllMain symbol [exporteddllmain]
-IGNORED-NOT: lld-link: warning: b.lib: skipping exported DllMain symbol [exporteddllmain]
+WARN: lld-link: warning: b.lib: skipping imported DllMain symbol [importeddllmain]
+IGNORED-NOT: lld-link: warning: b.lib: skipping imported DllMain symbol [importeddllmain]
DISASM: The Import Tables:
DISASM: DLL Name: b.dll
|
mstorsjo
reviewed
Jul 5, 2025
| // If an import library provides the DllMain symbol, skip importing it, as | ||
| // we should be using our own DllMain, not another DLL's DllMain. | ||
| if (sym.getName() == "__imp_DllMain" || sym.getName() == "DllMain" || | ||
| sym.getName() == "_DllMain") { |
There was a problem hiding this comment.
We shouldn't be testing two different syntaxes, we should only be testing the right one for each architecture. (Otherwise we'd be matching an unrelated symbol on x86_64 too, even if using a symbol named _DllMain there also would be kinda odd.) IIRC you can do e.g. mangle("DllMain") to get the right symbol name. And the same goes for the __imp_ form too, on i386 it would be __imp__DllMain, so Twine("__imp_") + mangle("DllMain") or something along those lines.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up PR for post-commit comments in #146610
DllMain.